Skip to content
This repository has been archived by the owner on Oct 19, 2024. It is now read-only.

fix(ethers_solc): hardcoded import remapping fix #2626

Merged
merged 2 commits into from
Oct 18, 2023

Conversation

plotchy
Copy link
Contributor

@plotchy plotchy commented Oct 6, 2023

Motivation

When using ethers_solc to compile contracts, remappings have an issue with the hardcoded specific fix for an openzeppelin contracts issue.

I'm not very familiar with the issue described in the doc comments, but it is erroring in situations where it shouldn't be applicable.

// we handle the edge case where the path of a remapping ends with "contracts"
// (`<name>/=.../contracts`) and the stripped import also starts with
// `contracts`
if let Ok(adjusted_import) = stripped_import.strip_prefix("contracts/") {
    if r.path.ends_with("contracts/") && !lib_path.exists() {
        return Path::new(&r.path).join(adjusted_import)
    }
}

Problematic setup

Example import in sol file:

import {Math} from "@openzeppelin/contracts/utils/math/Math.sol";

Example remapping:

@openzeppelin/=lib/openzeppelin-contracts/

in this case, the path of the remapping ends in "contracts/" (lib/openzeppelin-contracts/) and the stripped import also starts with "contracts" (contracts/utils/math/Math.sol).

this makes the resolver look at:
lib/openzeppelin-contracts/utils/math/Math.sol
when it should instead look at:
lib/openzeppelin-contracts/contracts/utils/math/Math.sol

Solution

Stripping based on .ends_with("contracts/") isn't precise. Better suited for issue regarding the comment is .ends_with("/contracts/").

In this instance, the remapping correctly would not get flagged, as it is does not end with "/contracts/" (lib/openzeppelin-contracts/).

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice find!

smol doc nit

@@ -348,7 +348,7 @@ impl ProjectPathsConfig {
// (`<name>/=.../contracts`) and the stripped import also starts with
// `contracts`
if let Ok(adjusted_import) = stripped_import.strip_prefix("contracts/") {
if r.path.ends_with("contracts/") && !lib_path.exists() {
if r.path.ends_with("/contracts/") && !lib_path.exists() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's add a note here with the openzeppelin-contracts as example

I need as much context as possible for all these rules

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok! Since it applies to more than oz, I wanted to keep it general.
we want to prevent matching on directories that end in things like -contracts, _contracts, .contracts

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you!

paging @DaniPopes

@DaniPopes DaniPopes merged commit d45deae into gakonst:master Oct 18, 2023
19 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants